-
Notifications
You must be signed in to change notification settings - Fork 40
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[KOGITO-9177] Introduce workflowproj module #143
Conversation
@@ -104,6 +105,7 @@ jobs: | |||
kubectl version | |||
kubectl get pods -A | |||
|
|||
# TODO: install the operator-sdk first, then cache the installation | |||
- name: Run tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are a few comments.
Gonna test this tomorrow morning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this filename change ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's a test case, I'll move it to testdata
once @desmax74's PR is rebased and merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, added few questions
// In dev mode means within the src/main/resources. In build contexts, the actual context dir. | ||
var externalResourceDestinationDir = map[metadata.ExtResType]string{ | ||
metadata.ExtResGeneric: "", | ||
metadata.ExtResCamel: workflowdef.ExternalResourceCamelMountDir, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's only for Camel or is just the first extension to be used and tested ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand your question. This map is to map (hehehe) the resource type and the target mount path. Except for camel, everything is mounted in src/main/resources
, which is a bug I intend to fix here: https://issues.redhat.com/browse/KOGITO-9263
|
||
package workflowproj | ||
|
||
// camelSchema version 3.20.5. We can update this schema as we go, or add support to more than one in the future. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the only way to load this schema ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could fetch from the internet, but CLIs that can't access external resources would fail to load. It's a fair question. If you have another idea, we could try in another PR, maybe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ricardozanini
In case of updates of the remote camel's file we have to change this file, isn't better to have a copy of this file external to the code, or put the content on a template file and load at the startup when is needed ? An external file is simple to edit and save instead with this approach we need to compile and redeploy an entire operator, image if only this file change, and we have to redeploy on the catalog/hub a new version/csv/olm/other configs files files only for a single line change on this. In the future this could be a separate module and the operator at the startup load the latest version without internal changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't we delegate this validation to the engine or to the quarkus-camel extension(idk if the extension validate is as well.)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't better to have a copy of this file external to the code, or put the content on a template file and load at the startup when is needed
In this use case, it's not. Usually we don't pack resources/templates in golang projects. Yet, this will be used by a CLI application. I'd say that's way better to align the Camel version upgrade on runtimes and also do it here using CI/automation instead.
Also, this is a corner case where we try to guess the contents of a file. It's not strict to the project, also it will change once I implement https://issues.redhat.com/browse/KOGITO-9263
image if only this file change, and we have to redeploy on the catalog/hub a new version/csv/olm/other configs files files only for a single line change on this
We will only change this if Kogito changes its Camel version, which require a new build/version of the whole platform. This will be just yet another change.
can't we delegate this validation to the engine or to the quarkus-camel extension(idk if the extension validate is as well.)?
Using quarkus/java here?
workflowproj/testdata/workflows/specs/workflow-service-schema.json
Outdated
Show resolved
Hide resolved
@@ -33,8 +33,7 @@ type KogitoServerlessWorkflowSpec struct { | |||
type KogitoServerlessWorkflowStatus struct { | |||
api.Status `json:",inline"` | |||
// +optional | |||
Address duckv1.Addressable `json:"address,omitempty"` | |||
Applied KogitoServerlessWorkflowSpec `json:"applied,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't more needed ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applied
should've been removed in your PR: https://github.com/kiegroup/kogito-serverless-operator/pull/128
@@ -37,6 +37,7 @@ func TestKogitoServerlessBuildController(t *testing.T) { | |||
WithRuntimeObjects(ksb, ksw). | |||
WithRuntimeObjects(test.GetKogitoServerlessPlatformInReadyPhase("../config/samples/"+test.KogitoServerlessPlatformWithCacheYamlCR, namespace)). | |||
WithRuntimeObjects(test.GetKogitoServerlessOperatorBuilderConfig("../", namespace)). | |||
WithStatusSubresource(ksb, ksw). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why WithStatusSubresource
istead of WithStatusSubResource
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a signature of the controller-runtime package: kubernetes-sigs/controller-runtime#2259
Signed-off-by: Ricardo Zanini <zanini@redhat.com>
Signed-off-by: Ricardo Zanini <zanini@redhat.com>
17c1a7c
to
8b5e4cd
Compare
workflowproj/workflowproj.go
Outdated
if err := w.parseRawProject(); err != nil { | ||
return err | ||
} | ||
if err := saveAsKubernetesManifest(w.project.Workflow, path); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check my comment on the PR
@ricardozanini thank you so much for this PR. One small change that I would like to include: We need to add null check if there is only the single file and no resources/app properties: My suggestion: workflowproj.go
A test to catch that:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to make sure that @ricardozanini address my comment before someone merge it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small comments, not mandatory.
Many thanks.
Signed-off-by: Ricardo Zanini <zanini@redhat.com>
Signed-off-by: Ricardo Zanini <zanini@redhat.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀 I'll keep you posted if I need anything else! Thank you so much
@davidesalerno since we are in a hurry to integrate with the CLI, I'm merging this one. If you spot anything that requires our attention, let me know so I can make the required adjustments. |
Description of the change:
This adds a new module to the project, so client code can create a whole Kubernetes Workflow project from local files or using any golang tooling.
api
package so users won't have to depend on the whole operatorworkflowproj
package, so users can programmatically create a new Kogito Workflow Kubernetes Deployment via the operatorSee
https://issues.redhat.com/browse/KOGITO-9177
Motivation for the change:
Adds the capability to convert simple Kogito SW files into a new devmode deployment.
Checklist
How to backport a pull request to a different branch?
In order to automatically create a backporting pull request please add one or more labels having the following format
backport-<branch-name>
, where<branch-name>
is the name of the branch where the pull request must be backported to (e.g.,backport-7.67.x
to backport the original PR to the7.67.x
branch).Once the original pull request is successfully merged, the automated action will create one backporting pull request per each label (with the previous format) that has been added.
If something goes wrong, the author will be notified and at this point a manual backporting is needed.